Skip to content

Restore relational/logical operators tests for sycl::vec#1059

Merged
bader merged 4 commits intoKhronosGroup:mainfrom
aelovikov-intel:restore-rel-logical
Mar 22, 2025
Merged

Restore relational/logical operators tests for sycl::vec#1059
bader merged 4 commits intoKhronosGroup:mainfrom
aelovikov-intel:restore-rel-logical

Conversation

@aelovikov-intel
Copy link
Copy Markdown
Contributor

@aelovikov-intel aelovikov-intel commented Feb 26, 2025 •
edited
Loading

Seems to have been removed accidentally in #130.
KhronosGroup/SYCL-Docs#675 might be considered somewhat related.

@aelovikov-intel aelovikov-intel requested a review from a team as a code owner February 26, 2025 19:49
aelovikov-intel added a commit to aelovikov-intel/SYCL-CTS that referenced this pull request Feb 26, 2025
@aelovikov-intel
Changes for generate_all_types_specific_return_type_test
b53712d 
Only meaningful after KhronosGroup#1059
keryell
keryell reviewed Mar 6, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something not implemented in SimSYCL?

@aelovikov-intel @keryell
Update tests/vector_operators/generate_vector_operators.py
924d6ca 
Co-authored-by: Ronan Keryell <ronan.keryell@amd.com>
@aelovikov-intel
Copy link
Copy Markdown
Contributor Author

aelovikov-intel commented Mar 18, 2025

Is this something not implemented in SimSYCL?

Based on the error, it's implemented incorrectly:

/__w/SYCL-CTS/SYCL-CTS/build/tests/vector_operators/vector_operators_bool.cpp:569:26: error: ambiguous overload for ‘operator=’ (operand types are ‘simsycl::sycl::vec<signed char, 1>’ and ‘simsycl::sycl::vec<bool, 1>’)
  569 |     resVec = testVec1 && testVec2;
      |                          ^~~~~~~~

Result of relational/logical operations should have fixed-width integer type of the same size as operand's element type, not bool. That's true in present version of the spec, but is clarified further in the proposed KhronosGroup/SYCL-Docs#675.

keryell
keryell approved these changes Mar 19, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Is the SimSYCL team watching this?

bader
bader requested changes Mar 19, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, disable failing tests for SimSYCL implementation. PR must not regress CI checks.

https://github.com/KhronosGroup/SYCL-CTS/tree/main/docs#compile-time-disabled-test-cases

@aelovikov-intel aelovikov-intel requested a review from bader March 19, 2025 18:26
bader
bader approved these changes Mar 19, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@bader bader merged commit 1d48dfe into KhronosGroup:main Mar 22, 2025
9 checks passed
@aelovikov-intel aelovikov-intel deleted the restore-rel-logical branch March 26, 2025 16:55
@tomdeakin tomdeakin mentioned this pull request Mar 27, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bader bader bader approved these changes

@keryell keryell keryell approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aelovikov-intel @bader @keryell